-
Notifications
You must be signed in to change notification settings - Fork 773
Conversation
Signed-off-by: lowzj <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #926 +/- ##
==========================================
- Coverage 46.75% 46.75% -0.01%
==========================================
Files 112 112
Lines 6651 6649 -2
==========================================
- Hits 3110 3109 -1
Misses 3291 3291
+ Partials 250 249 -1
Continue to review full report at Codecov.
|
9f479a6
to
de058d6
Compare
What will happen if the log file exceed the max file size 40MB? |
logrus.Warnf("request piece result:%v", response) | ||
if code == constants.CodeSourceError { | ||
p2p.cfg.BackSourceReason = config.BackSourceReasonSourceError | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant else
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not redundant. It makes the warn log
only outputted when getting an unexpected response code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. Sorry for not have a look at the code carefully.
@@ -61,7 +61,7 @@ func WithLogFile(f string) Option { | |||
if logger := getLumberjack(l); logger == nil { | |||
l.SetOutput(&lumberjack.Logger{ | |||
Filename: f, | |||
MaxSize: 20, // mb | |||
MaxSize: 40, // mb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it configurable? WDYT? And so do with the MaxBackups
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree. There's already a function named WithMaxSizeMB
to configure MaxSize
. It's better to open a new pr to implement this especially make the supernode's log configurable.
LGTM. |
@yeya24 FYI. https://github.com/natefinch/lumberjack |
log: not rotate log file when startup
log: not rotate log file when startup
Signed-off-by: Jim Ma <[email protected]>
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
fixes part of : #921
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews